Codecov Report

Merging #503 (cd68e87) into main (9ed18ca) will decrease coverage by 1.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main     #503      +/-   ##
==========================================
- Coverage   67.96%   66.94%   -1.02%     
==========================================
  Files          56       67      +11     
  Lines        4117     4396     +279     
==========================================
+ Hits         2798     2943     +145     
- Misses       1319     1453     +134     

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ed18ca...cd68e87. Read the comment docs.

It would be cool, if anyone could have a look at this PR. In the metrics_test.py file, I propose an updated version of the c2st function which runs considerably faster than the current implementation. Moreover, the choice of classifyer is flexible. If people like @michaeldeistler @janfb agree, I'd go forward and swap alt_c2st in the unit test file with the one in sbi/utils/metrics.py and remove the Draft label.

Thanks for the ping @psteinb !

I think this is very valuable input you give 👍 , but I would need to look into it a bit, which could happen next week the earliest.

Also, I think it would make sense to @jan-matthis input on this topic.

sure thing, I just found some time today to invest in this. The difference in runtime for the test cases I could conceive (and there are likely more) is striking. Looking forward to your feedback!

I merged main into this branch, so that the CI passes.

Just wanting to pick this up here.

@janfb and @jan-matthis should I update this PR and bring the randomforest based c2st into sbi by replacing the old NN based implementation?

This version of c2st is likely to speed up some unit tests, see also #575. I am currently studying the effect of using a RF instead of an MLP here

As suggested by @janfb, I ran a small analysis on a synthetic dataset with c2st using a NN (as is currently done) versus c2st using a random forest (with sklearns default parameters). For the complete details of this analysis, consult this notebook.

Bottom line for me:

• the rf based implementation gives approximately the same degree of results than the NN version
• the speedup using a rf can be upto 32x

@janfb or anyone else ready for review.

1. Is there a reason to change the default classifier?

Depends on the input data. There is no free lunch so I thought, having the classifier be configurable makes sense.

2. Overall, these new tests should not exceed five minutes (3 minutes would be even better) (otherwise our total test time will explode in the long run). Could you check this and remove tests if necessary?

Fair enough, I'll look into this. Based on my analysis here using a smaller dataset size should be fine. Those 3 minutes apply to all non-slow tests?

Aside: all tests are failing atm because of a missing import

I'll see to it.

Yes, making it configurable is great. But I would stick to the MLP as default.

3-5 minutes applies to all tests, including slow tests. (sorry about being picky here, but I really actively trying to keep test time managable)

Thanks a lot!

thanks, this is great!

I think the speed up we gain from using random forest vs MLP is a very strong argument for changing the defaults, @michaeldeistler . You do not? What is your argument for keeping the MLP classifier?

Yes I'm perfectly fine with swapping out the classifier default. But please run all tests (including slow) and make sure that they pass. Thanks!

Thanks for the feedback. I introduced early stopping for the MLP classifier and am currently rerunning my mini-study. It does reduce the MLP runtime considerably iff the two ensembles are (far) apart from each other and are easy to classify. If that is not the case, the runtime is still rather long. All tests pass on my machine and in the CI. Runtimes look good to me:

============================================================================= slowest durations =============================================================================
4.09s call     tests/metrics_test.py::test_same_distributions_mlp
3.92s call     tests/metrics_test.py::test_diff_distributions_flexible_mlp
2.00s call     tests/metrics_test.py::test_same_distributions
1.56s call     tests/metrics_test.py::test_c2st_scores
1.42s call     tests/metrics_test.py::test_onesigma_apart_distributions
0.63s call     tests/metrics_test.py::test_diff_distributions

I rerequested the review.

I think it would be great to simplify the tests, and to simplify c2st(..) to justify the separation from c2st_scores (see new comment above!).

Then I would also let @michaeldeistler look at this again, and finally I think it would be great if you could do an interactive rebase and group these rather many commits to obtain just a couple of commits. Thanks!

Thanks for improving the tests! Great effort 🚀

You want me to squash those commits in this PR and rebase onto main? I am totally happy if you want to just squash-merge. Just let me know.

No, it's like it is if we squash-merge. only if we were to rebase and merge then I would prefer some rewriting of the commits.
👍

Horray! 🕺